Stamp per-worker-process execution metadata on test results - #420

From River:

Reviewed at 3384eab. Nice change overall — the fork-reset mirrors the existing FileLoader#detect_fork! pattern, the accessors are nil-safe for embedders that don't have them, the new keys are purely additive, and the first-writer-wins contract has real test coverage. A few things I'd want addressed before this lands.

Blocking-ish

1. Thread-based parallelization silently produces wrong data

All three pieces of state — @parallel_worker_next_test_index, the pid, and parallel_worker_id — live on the Minitest::Queue singleton, so they are process-global.

Under thread-based parallelization (Rails parallelize(workers: N, with: :threads)) every "worker" shares one pid and one parallel_worker_id. The documented partition key:

PARTITION BY job_id, parallel_worker_id, parallel_worker_pid
ORDER BY parallel_worker_test_index

then collapses to a single partition, and the indexes interleave across threads. The result still looks like a valid per-worker execution order downstream, which is the worst failure mode for a telemetry field — silently wrong beats obviously missing. @parallel_worker_next_test_index += 1 also isn't atomic once you're off CRuby's GVL (JRuby/TruffleRuby, where :threads mode is most likely to be used).

Two ways out, either is fine:

• Key the counter (and ideally the worker id) on Thread.current rather than the module.
• Or state plainly in the README that only fork-based parallelization is supported, and that parallel_worker_* must not be trusted under with: :threads.

2. The version bump doesn't belong in this PR

Commit 3384eab bumps to 0.98.0. The repo's own Releasing a New Version section says to bump after merging changes to main, and the history backs that up — #419 was a standalone Bump ci-queue to v0.97.0. Bundling it here also guarantees a conflict with any other in-flight PR that does the same. Suggest dropping the commit and doing the release separately.

Worth fixing

3. ENV is re-read and re-parsed on every test

parallel_worker_id calls parallel_worker_id_from_env on every stamp, i.e. once per test execution. With a non-numeric value (CI_QUEUE_PARALLEL_WORKER_ID=worker-a, explicitly supported per the tests) that's an Integer() raise plus rescue per test. This repo has previously shaved per-heartbeat Symbol#to_s allocations, so a per-test exception is going to get noticed.

Memoize it, and clear the memo in the same pid-change branch that already resets the counter — that keeps it correct across forks, where a worker id injected post-fork must not be shadowed by the parent's value:

def parallel_worker_id
  return @parallel_worker_id if @parallel_worker_id

  @parallel_worker_id_from_env = parallel_worker_id_from_env unless defined?(@parallel_worker_id_from_env)
  @parallel_worker_id_from_env
end

4. The payload isn't self-sufficient for the query that justifies it

The partition key in the description starts with job_id, but test_data.json carries neither job_id nor ci-queue's own worker_id — TestData#to_h has no worker or build identity at all. So "a SQL query can reproduce any worker's test_order-w{N}-{pid}.log" holds only if the ingestion layer attaches job_id out of band. Worth either emitting queue.config.worker_id alongside the new fields, or saying explicitly in the README where job_id is expected to come from.

5. Naming collides with ci-queue's existing worker_id

ci-queue already has a first-class worker_id: config.worker_id, set by --worker / inferred from the CI provider, used throughout CI::Queue::Redis::Worker, and already emitted in the worker profile payload. That's a whole queue worker. The new parallel_worker_id is a fork inside one of those. Two different "worker" concepts landing in the same telemetry pipeline will get conflated by whoever writes the queries. Not a blocker, but consider something like intra_worker_* / process_worker_*, or at minimum call out the relationship in the README.

Nits

• README placement. The new ### Parallel worker metadata section sits right after the deprecated RSpec section, but the feature is minitest-only (rspec-queue is untouched). It belongs under ### Minitest, and the one-row env table would be better folded into the existing CI_QUEUE_* table there rather than starting a second one.
• README accuracy. "the pid of the worker process at result creation" — it's actually stamped at the top of handle_test_result, after the test has run. Same process in the in-process flow, so the value is right, but the wording matters precisely because the DRb contract hinges on when stamping happens.
• test_ignores_results_without_accessors asserts nothing. The comment says "does not raise", but nothing enforces that intent. assert_nil Minitest::Queue.stamp_parallel_worker_metadata(plain) (or assert_silent) would make the test fail for the right reason.
• Unit tests leak global state. teardown restores @parallel_worker_id and the env var, but not @parallel_worker_metadata_pid / @parallel_worker_next_test_index — and test_index_restarts_when_pid_changes deliberately mutates the former. Harmless today because every other assertion is relative, but it's a trap for the next test added to this file.
• assert_equal (0...failures.size).to_a, ... in the integration test — the space before the paren makes Ruby parse it as a grouped expression; worth closing up.

Note on CI

There are no workflow runs on head SHA 3384eab — only Graphite's mergeability_check reports, and the combined status is pending. So the suite hasn't actually been exercised on this branch yet.

Review requested by Mike Wilson mike.wilson@shopify.com via River.
Slack thread: https://shopify.slack.com/archives/C0BL3BCFL4A/p1786462080803919

Thanks for the thorough review — addressed in c6d40a8 (and a force-push dropping the version bump). Point by point:

1. Thread-based parallelization — Agreed, and it's actually nearer-term than with: :threads: DRb servers dispatch each incoming call on its own thread, so an embedder's reporting process could race the counter even on CRuby. Took the fix rather than the disclaimer for atomicity: stamp state is now mutex-guarded (with a thread-safety test), plus a README note that per-worker order reconstruction is only meaningful with forked workers — under threads, everything shares one (worker_id, pid) partition and the index is record order across threads. I didn't take the Thread.current option: thread identity isn't a stable partition key, there's no thread-id field in the schema to carry it, and per-thread counters would produce colliding (pid, index) pairs within a process.

2. Version bump — Fair, that's the documented release flow and the #419 precedent. Dropped from this PR; the bump is parked on a branch and will go up as a standalone PR once this merges.

3. ENV re-read per test — Fixed. Memoized keyed on Process.pid (rather than clearing in the stamp's pid-change branch) so the getter is fork-safe even when called outside stamping. Documented as "read once per process"; test added.

4. Payload self-sufficiency — Documented rather than emitted. test_data.json has never carried build/job identity for any field — attaching it is the ingestion layer's job (e.g. Core's wrapper merges buildkite_job_id), and emitting queue.config.worker_id from TestData would mean plumbing queue access into a class that deliberately has none, duplicating what the pipeline already does. README now states where job_id is expected to come from.

5. Naming — Keeping parallel_worker_*, but the relationship is now called out in the README ("a forked test process inside one queue worker; unrelated to --worker/config.worker_id"). Renaming isn't really on the table: shopify_build_test_results/3.3 is already merged with these exact field names, and Rails 8.1's ActiveSupport::TestCase.parallel_worker_id uses the same term for the same concept, so this matches the ecosystem the main embedder lives in.

Nits — All taken: section moved under Minitest (kept its own small env table — the env tables are per-feature in this README), stamp-timing wording fixed, stamp_parallel_worker_metadata now returns the result or nil so the skip paths are assertable (assert_nil in both the no-accessor and pre-stamped tests), all five stamp-state ivars reset in setup/teardown, paren spacing fixed.

CI — the missing runs were the GitHub Actions outage earlier today; the force-push should re-trigger.

AI generated (pi/claude, reviewed by @aiden before posting)